suggestions #7382
Merged
suggestions #7382
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matthieusieben
commented
Jan 7, 2025
| @@ -16,9 +16,9 @@ export * as profile from '#/types/atproto/profile' | |||
| * } | |||
| * ``` | |||
| */ | |||
| export function fastIsType<R>( | |||
| export function fastIsType<R extends {$type?: string}>( | |||
There was a problem hiding this comment.
This change prevents using fastIsType with a type param R that does not match the predicate function.
Comment on lines
261
to
265
| // Type casting here is actually safe because here we check for the | ||
| // actual type bellow | ||
| const record = notif.record as | ||
| | AppBskyFeedRepost.Record | ||
| | AppBskyFeedLike.Record |
There was a problem hiding this comment.
Totally! This is actually what fastIsType is doing as well, just casting the values without validation. I decided to make the util so that it's clear what's happening when that util is used, instead of feeling the need to leave a comment on each type cast as you did here. That seem reasonable to you?
Comment on lines
+19
to
+21
| export function fastIsType<R extends {$type?: string}>( | ||
| record: unknown, | ||
| identity: <V>(v: V) => boolean, | ||
| identity: <V>(v: V) => v is V & {$type: NonNullable<R['$type']>}, |
|
src/components/MediaPreview.tsx
Outdated
Comment on lines
26
to
31
| embed?: | ||
| | $Typed<AppBskyEmbedRecordWithMedia.View> | ||
| | $Typed<AppBskyEmbedImages.View> | ||
| | $Typed<AppBskyEmbedVideo.View> | ||
| | $Typed<AppBskyEmbedExternal.View> | ||
| | {$type: string} |
There was a problem hiding this comment.
I get why this makes sense and is probably the most academic way of doing this, but a few things I don't like:
- We lose the relationship between the prop
embedand the origin of the data inPostView['embed']. The way it was typed before makes it very obvious how this component is used - Having to remember to duplicate the forwards compat via
{$type: string}in components like this is boilerplate I don't think we want. If we adopt this pattern, we'll be doing this in hundreds of places. - Having to reconstruct these types exactly but understanding exactly what can be passed in here (plus needing the $Type util) requires understanding of the data on both sides of this component. This makes it more cumbersome to make "dumb" components that can be composed and reused throughout the app.
estrattonbailey
added a commit
that referenced
this pull request
Jan 8, 2025
* suggestions * Revert unneeded changes --------- Co-authored-by: Eric Bailey <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.